[internal/hcs] Migrate package from HCS V1 to V2#2735
Conversation
rawahars
force-pushed
the
migrate_hcs_v1_v2
branch
15 times, most recently
from
54b7a44 to
d3f62f2
Compare
rawahars
force-pushed
the
migrate_hcs_v1_v2
branch
3 times, most recently
from
adb6ee4 to
c399db9
Compare
Presently, we were using HCS V1 (vmcompute) within internal/hcs but it has been deprecated and therefore, we are moving to use computecore within hcs package. Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
rawahars
force-pushed
the
migrate_hcs_v1_v2
branch
from
c399db9 to
e76ebeb
Compare
helsaawy
left a comment
helsaawy
left a comment
There was a problem hiding this comment.
this change will cause the shim to always block on all operations, right? do we have any perf numbers on how much impact this will have?
| closeOnce sync.Once | ||
| exited chan struct{} | ||
| closed chan struct{} | ||
| raw string |
There was a problem hiding this comment.
might be cleaner to use:
| raw string | |
| raw json.RawMessage |
| // owns the operation handle leads to use-after-free crashes | ||
| // (EXCEPTION_ACCESS_VIOLATION) inside computecore.dll. Callers must not | ||
| // rely on ctx to bound the call's duration. | ||
| func runOperation(ctx context.Context, fn func(op computecore.HcsOperation) error) (resultDoc string, err error) { |
There was a problem hiding this comment.
we always end up calling processHcsResult on the operation result; ie:
resultJSON, err := runOperation(ctx, func(op computecore.HcsOperation) error { /* ... */ })
if err != nil {
return nil, makeSystemError(computeSystem, operation, err, processHcsResult(ctx, resultJSON))
}can we elevate hcsResult to an error type (since it already (sorta) matches the ResultError API type), and then move the processHcsResult logic into run[Process]Operation?
that would reduce a lot of boilerplate
| id := registerNotificationContext(computeSystem.id, 0, computeSystem.notify, computeSystem.migrationNotifyCh) | ||
| if err := computecore.HcsSetComputeSystemCallback( | ||
| ctx, computeSystem.handle, | ||
| computecore.HcsEventOptionEnableOperationCallbacks|computecore.HcsEventOptionEnableLiveMigrationEvents, |
There was a problem hiding this comment.
notificationHandler doesn't actually handle the operation events (and we don't keep track of the actively running operations anywhere), so it the HcsEventOptionEnableOperationCallbacks flag needed?
4 participants
Presently, we were using HCS V1 (vmcompute) within internal/hcs but it has been deprecated and therefore, we are moving to use computecore within hcs package.